These are the created ipsets:

# ipset list
...
Name: kube-router-ipvs-services
Type: hash:ip,port
Revision: 5
Header: family inet hashsize 1024 maxelem 65536 timeout 0
Size in memory: 512
References: 1
Members:
10.96.0.10,tcp:53 timeout 0
10.111.170.143,tcp:80 timeout 0
10.96.0.1,tcp:443 timeout 0
10.96.0.10,udp:53 timeout 0

Name: kube-router-service-ips
Type: hash:ip
Revision: 4
Header: family inet hashsize 1024 maxelem 65536 timeout 0
Size in memory: 416
References: 1
Members:
10.96.0.1 timeout 0
10.96.0.10 timeout 0
10.111.170.143 timeout 0

These are the created iptables rules:

# iptables --list-rules
-P INPUT ACCEPT
-P FORWARD DROP
-P OUTPUT ACCEPT
...
-N KUBE-ROUTER-SERVICES
-A INPUT -m comment --comment "handle traffic to IPVS service IPs in custom chain" -m set --match-set kube-router-service-ips dst -j KUBE-ROUTER-SERVICES
...
-A KUBE-ROUTER-SERVICES -m comment --comment "allow input traffic to ipvs services" -m set --match-set kube-router-ipvs-services dst,dst -j ACCEPT
-A KUBE-ROUTER-SERVICES -p icmp -m comment --comment "allow icmp echo requests to service IPs" -m icmp --icmp-type 8 -j ACCEPT
-A KUBE-ROUTER-SERVICES -m comment --comment "reject all unexpected traffic to service IPs" -j REJECT --reject-with icmp-port-unreachable

You need to filter out node IP somehow, because adding NodePort service will reject all other traffic to node IP. Also maybe it is better to insert rule to INPUT chain rather than append because firewalld adds final REJECT all rule and there is no use appending our rules after it.

@bazuchan thanks for the input. I don't use NodePort, so that was not on my radar.

You need to filter out node IP somehow, because adding NodePort service will reject all other traffic to node IP. Also maybe it is better to insert rule to INPUT chain rather than append because firewalld adds final REJECT all rule and there is no use appending our rules after it.

If I add our rules to a custom chain and make sure all relevant traffic goes there then I should be independent of what firewalld does, right?

I will setup a test bed with all possible service types (ClusterIP, LoadBalancer, NodePort) and investigate what's needed.

Actually NodePort services themselves seem to be handled/whitelisted just fine by the PR.

BUT as soon as you have a NodePort service, that causes the nodes IP to be part of the IPVS services, which means they get added to the kube-router-service-ips ipset. This again causes all traffic to the nodes IPs that is not explicitly allowed to be rejected, including e.g. access to the kubernetes API.

If I change my REJECT rule from:

iptables -A KUBE-ROUTER-SERVICES -m comment --comment "reject all unexpected traffic to service IPs" -j REJECT --reject-with icmp-port-unreachable

to

iptables -A KUBE-ROUTER-SERVICES -m comment --comment "reject all unexpected traffic to service IPs" \ 
   -m set ! --match-set kube-router-node-ips dst \
   -j REJECT --reject-with icmp-port-unreachable

Then NodePort services are covered and other traffic to node local IPs still work.

The ipset kube-router-node-ips contains the nodes local IPs and is created by pkg/controllers/routing/network_routes_controller.go, so it's only created if kube-router is run with routing enabled.

@murali-reddy, @roffe Would it make sense to move all the ipset handling out of the current controllers to some dedicated place (another controller?) and always create and manage them no matter which parts of kube-router are actually active? Otherwise I have to replicate what the kube-router-node-ips ipset does in the proxy controller. Other ideas?

@asteven thanks for working on this. I will take a look and test it out.

Would it make sense to move all the ipset handling out of the current controllers to some dedicated place (another controller?) and always create and manage them no matter which parts of kube-router are actually active? Otherwise I have to replicate what the kube-router-node-ips ipset does in the proxy controller.

Each controller has its own set of ipsets that they manage. I would prefer to have no coupling at all between the controllers or dependency of other components to the extent possible. Basically keep them self contained except for utility functions or cross-cutting concerns like metrics, health/liveness checks etc.

@murali-reddy Ok, so I'll update the PR to manage its own ipset for node IPs. Do you have a simple way to run something like end to end tests? e.g. how to test the DSR related code? It currently seems difficult to change something without the risk of breaking something else that you're not using/looking out for.

@murali-reddy, @bazuchan I'm using this in my staging environment and it works for me. Can you please review the PR?

For reference, these are the ipsets and firewall rules created by the PR:

nodeIP: 10.206.32.10
NodePort service: 10.206.32.10,tcp:30080

All other listed services are either ExternalIP or just plain ClusterIP.

# ipset list
Name: kube-router-local-ips
Type: hash:ip
Revision: 4
Header: family inet hashsize 1024 maxelem 65536 timeout 0
Size in memory: 416
References: 1
Members:
10.206.32.10 timeout 0
XXX.XXX.174.108 timeout 0
127.0.0.1 timeout 0

Name: kube-router-service-ips
Type: hash:ip
Revision: 4
Header: family inet hashsize 1024 maxelem 65536 timeout 0
Size in memory: 608
References: 1
Members:
10.107.89.254 timeout 0
10.96.0.10 timeout 0
10.206.32.10 timeout 0
10.96.0.1 timeout 0
10.107.193.191 timeout 0

Name: kube-router-ipvs-services
Type: hash:ip,port
Revision: 5
Header: family inet hashsize 1024 maxelem 65536 timeout 0
Size in memory: 704
References: 1
Members:
10.96.0.10,udp:53 timeout 0
10.96.0.10,tcp:53 timeout 0
10.96.0.1,tcp:443 timeout 0
10.107.193.191,tcp:80 timeout 0
10.206.32.10,tcp:30080 timeout 0
10.107.89.254,tcp:80 timeout 0

# iptables --list-rules
-P INPUT ACCEPT
-P FORWARD DROP
-P OUTPUT ACCEPT
-N DOCKER
-N DOCKER-ISOLATION
-N KUBE-FIREWALL
-N KUBE-ROUTER-SERVICES
-A INPUT -m comment --comment "handle traffic to IPVS service IPs in custom chain" -m set --match-set kube-router-service-ips dst -j KUBE-ROUTER-SERVICES
-A INPUT -j KUBE-FIREWALL
-A FORWARD -o service -m comment --comment "allow outbound node port traffic on node interface with which node ip is associated" -j ACCEPT
-A FORWARD -o kube-bridge -m comment --comment "allow inbound traffic to pods" -j ACCEPT
-A FORWARD -i kube-bridge -m comment --comment "allow outbound traffic from pods" -j ACCEPT
-A FORWARD -j DOCKER-ISOLATION
-A FORWARD -o docker0 -j DOCKER
-A FORWARD -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
-A FORWARD -i docker0 ! -o docker0 -j ACCEPT
-A FORWARD -i docker0 -o docker0 -j ACCEPT
-A OUTPUT -j KUBE-FIREWALL
-A DOCKER-ISOLATION -j RETURN
-A KUBE-FIREWALL -m comment --comment "kubernetes firewall for dropping marked packets" -m mark --mark 0x8000/0x8000 -j DROP
-A KUBE-ROUTER-SERVICES -m comment --comment "allow input traffic to ipvs services" -m set --match-set kube-router-ipvs-services dst,dst -j ACCEPT
-A KUBE-ROUTER-SERVICES -p icmp -m comment --comment "allow icmp echo requests to service IPs" -m icmp --icmp-type 8 -j ACCEPT
-A KUBE-ROUTER-SERVICES -m comment --comment "reject all unexpected traffic to service IPs" -m set ! --match-set kube-router-local-ips dst -j REJECT --reject-with icmp-port-unreachable

Sorry for the delay @asteven I will do it tomorrow.

thanks @asteven for your work on this important functionality.

I gave first pass and testing out. Looks good to me overall.

If i have to summarise the critical change is:

• rule to jump the traffic destined for all service VIP to KUBE-ROUTER-SERVICES

# iptables -t filter -L -n -v
Chain INPUT (policy ACCEPT 209 packets, 83518 bytes)
 pkts bytes target     prot opt in     out     source               destination         
29514   17M KUBE-ROUTER-SERVICES  all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* handle traffic to IPVS service IPs in custom chain */ match-set kube-router-service-ips dst

• and rule to permit only to the intended ports for service vip's (cluster IP, external IP's)

# iptables -t filter -L KUBE-ROUTER-SERVICES -n -v
Chain KUBE-ROUTER-SERVICES (1 references)
 pkts bytes target     prot opt in     out     source               destination         
    0     0 ACCEPT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* allow input traffic to ipvs services */ match-set kube-router-ipvs-services dst,dst
    0     0 ACCEPT     icmp --  *      *       0.0.0.0/0            0.0.0.0/0            /* allow icmp echo requests to service IPs */ icmptype 8
    0     0 REJECT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* reject all unexpected traffic to service IPs */ ! match-set kube-router-local-ips dst reject-with icmp-port-unreachable

third rule is i believe to exclude the local Ip's, as this is something kube-router should not be handling explictly?

I will do further testing if the ipsets are synced properly on service add/delete events.

If i have to summarise the critical change is:

* rule to jump the traffic destined for all service VIP to KUBE-ROUTER-SERVICES

Yes. I went for the custom chain as that's easier to order the rules and to cleanup.

* and rule to permit only to the intended ports for service vip's (cluster IP, external IP's)

Yes. Whitelist all expected ip:port combinations (cluster IP, external IP's and NodePort)

# iptables -t filter -L KUBE-ROUTER-SERVICES -n -v
Chain KUBE-ROUTER-SERVICES (1 references)
 pkts bytes target     prot opt in     out     source               destination         
    0     0 ACCEPT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* allow input traffic to ipvs services */ match-set kube-router-ipvs-services dst,dst
    0     0 ACCEPT     icmp --  *      *       0.0.0.0/0            0.0.0.0/0            /* allow icmp echo requests to service IPs */ icmptype 8
    0     0 REJECT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* reject all unexpected traffic to service IPs */ ! match-set kube-router-local-ips dst reject-with icmp-port-unreachable

third rule is i believe to exclude the local Ip's, as this is something kube-router should not be handling explictly?

Yes. If we don't exclude the nodes local IPs then all traffic to any of them which is part of a ipvs service is blocked. This is mainly needed when NodePort services are used, as in that case the traffic directed to the nodes IP enters our custom chain.

I'm not sure if the 'allow icmp echo requests to service IPs' rule makes sense or is even potentially misleading.

BTW: all of this does not support IPv6 yet. But I think that should be addressed in some future PR.

Changes look good to me from code-review and testing.

BTW: all of this does not support IPv6 yet. But I think that should be addressed in some future PR.

@asteven Could you please open an issue for IPv6 so that its in the backlog.